New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
local-up-cluster.sh: install CSI snapshotter #91504
local-up-cluster.sh: install CSI snapshotter #91504
Conversation
/ cc @xing-yang |
${KUBECTL} --kubeconfig="${CERT_DIR}/admin.kubeconfig" apply -f "${KUBE_ROOT}/cluster/addons/volumesnapshots/volume-snapshot-controller/rbac-volume-snapshot-controller.yaml" | ||
${KUBECTL} --kubeconfig="${CERT_DIR}/admin.kubeconfig" apply -f "${KUBE_ROOT}/cluster/addons/volumesnapshots/volume-snapshot-controller/volume-snapshot-controller-deployment.yaml" | ||
|
||
echo "Kubernetes-CSI snapshotter successfully deployed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if deployment is started successfully before printing out this message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same but then followed the example set by the dashboard installation which also doesn't wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose "deployed" really means just "deployed" and not also "running".
/retest |
/retest pull-kubernetes-e2e-kind timed out. |
/retest Another unrelated error ("CronJob should remove from active list jobs that have been deleted expand_more") in pull-kubernetes-e2e-kind. |
hack/local-up-cluster.sh
Outdated
@@ -90,6 +90,9 @@ PRESERVE_ETCD="${PRESERVE_ETCD:-false}" | |||
# enable kubernetes dashboard | |||
ENABLE_CLUSTER_DASHBOARD=${KUBE_ENABLE_CLUSTER_DASHBOARD:-false} | |||
|
|||
# enable Kubernetes-CSI snapshotter | |||
ENABLE_CSI_SNAPSHOTTER=${ENABLE_CSI_SNAPSHOTTER:-true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pohly do you mind switching this to off as default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because most users of the script won't need it?
That's okay for me, I can always just enable it via the command line snippet that I use for invoking the script. I am less sure about users who need it but don't know about it yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, force-pushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this.
ab9c36a
to
d73d153
Compare
This cluster add-on is required for snapshotting of CSI volumes and must be installed when bringing up a cluster because CSI driver installations depend on that. It is unclear how many users of the script need CSI snapshotting, therefore it is disabled by default (= the previous behavior).
d73d153
to
2a31764
Compare
/retest |
3 similar comments
/retest |
/retest |
/retest |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
what other clusters should be installing this? |
bonus: how are we going to manage this when we push all of the cluster bringup code out-of-tree? |
We have documented that the CRDs and snapshot controller installations are the responsibility of the Kubernetes distribution. https://kubernetes.io/docs/concepts/storage/volume-snapshots/#introduction @msau42 also sent out an email about it: |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This cluster add-on is required for snapshotting of CSI volumes and
must be installed when bringing up a cluster because CSI driver
installations depend on that.
Does this PR introduce a user-facing change?:
/sig storage